Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor url assertions #1

Merged
merged 9 commits into from
Feb 29, 2024
Merged

Refactor url assertions #1

merged 9 commits into from
Feb 29, 2024

Conversation

wlandau
Copy link
Member

@wlandau wlandau commented Feb 29, 2024

@shikokuchuo, I read what you did in the last couple commits, and I really like it. In the future though, it would really help to submit pull requests when we make changes to parts of r-releases, especially this package.

This PR is a refactor and enhancement of URL assertions. In particular:

  • It adds more strict checks on the URL, including the use of https and use of a gitlab/github domain.
  • It simplifies assert_package() to check the URL and name instead of having to read a text file.
  • It renames assert_package_url() to assert_cran_url(), simplifies the code (at least in my eyes), and aligns it to the rest of the package.
  • It calls assert_cran_url() from assert_package_url() to make review_pull_request() simpler.

I would strongly prefer to avoid modifying review_pull_request() and review_pull_requests() as much as possible because these functions call the API and are much harder to test. This PR reverts review_pull_request() back to pretty much the way it was, except assert_package_contents() is now replaced by assert_package().

@wlandau wlandau self-assigned this Feb 29, 2024
@wlandau
Copy link
Member Author

wlandau commented Feb 29, 2024

Also, I replaced pkgsearch::ps() with the more specialized pkgsearch::cran_package(). I noticed issues with the former, e.g. ps("crew", size = 1) returns info on crew.aws.batch.

@wlandau
Copy link
Member Author

wlandau commented Feb 29, 2024

Also: I was using shaky grep patterns before this PR, and I really appreciate being able to use nanonext::parse_url().

@wlandau
Copy link
Member Author

wlandau commented Feb 29, 2024

I also added a check to flag manual review if a user contributes a URL which uses the CRAN mirror https://github.com/cran/

@shikokuchuo
Copy link
Member

Also, I replaced pkgsearch::ps() with the more specialized pkgsearch::cran_package(). I noticed issues with the former, e.g. ps("crew", size = 1) returns info on crew.aws.batch.

This is great - much more robust! Yes, the changes I put through were meant to add the functionality quickly - I'm sure a lot can be optimized down this road.

Copy link
Member

@shikokuchuo shikokuchuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good subject to my comments. I don't need to review again before merging if just these.

R/assert_cran_url.R Outdated Show resolved Hide resolved
R/assert_cran_url.R Show resolved Hide resolved
R/assert_cran_url.R Outdated Show resolved Hide resolved
R/assert_package.R Outdated Show resolved Hide resolved
R/assert_package.R Outdated Show resolved Hide resolved
R/assert_package.R Outdated Show resolved Hide resolved
R/build_universe.R Show resolved Hide resolved
@wlandau
Copy link
Member Author

wlandau commented Feb 29, 2024

Thanks for your review, @shikokuchuo. I believe I addressed all your comments.

@shikokuchuo
Copy link
Member

LGTM

@wlandau wlandau merged commit 5feb1a2 into r-multiverse:main Feb 29, 2024
2 checks passed
@wlandau wlandau deleted the url-assertions branch February 29, 2024 11:54
wlandau added a commit that referenced this pull request Aug 21, 2024
use calendar month for staging period
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants